fix: harden credential and runtime trust boundaries (rebase of #916) - #936
fix: harden credential and runtime trust boundaries (rebase of #916)#936lidge-jun wants to merge 4 commits into
Conversation
Review of this branch rejected three things. Two are docs; this is the behavior one. With no trusted launcher context, the branch deleted all three ambient Anthropic slots. That breaks a documented entry point: `bun src/cli/index.ts` is supported (structure/01_runtime.md:9) and has no launcher context, so a user who exported ANTHROPIC_API_KEY in their shell simply loses it. The two slot classes are not symmetric. ANTHROPIC_BASE_URL stays fail-closed — a dotenv-only destination combined with subscription auth is exactly how Claude's OAuth bearer and prompt leave for a host the repository chose, and losing a legitimate custom destination costs a flag rather than an account. Credentials are preserved: the destination is already pinned by the time they are read, so stripping them defends against a project file that could equally well have supplied the key it is being blamed for. The test that mandated the old behavior is replaced by two: an ambient key survives without context, and an ambient base URL is still replaced. That pair is the contract. Also corrected two structure docs the review flagged as stale: the ci.yml row still described the pre-#899 hosted-Windows selector, and the runtime decision log recorded the rejected fail-closed-for-everything behavior.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
The security review rejected my narrowing, correctly. I had preserved ambient credentials when no launcher context exists, reasoning that the destination is pinned before they are read so a dotenv key would only reach the local proxy. That reasoning does not survive the subscription path: CLAUDE_CODE_PROVIDER_MANAGED_BY_HOST is only set when opencodex owns an auth token — asserting it otherwise logs a subscriber out (#253) — so Claude Code's settings.env merge can still replace ANTHROPIC_BASE_URL after buildClaudeEnv returns. A preserved key travels to that host. The repository documents the destination residual for subscription mode; preserving credentials would have widened it into a credential leak. So all three slots fail closed again without provenance. Direct `bun src/cli/index.ts` loses ambient Anthropic values, which is a real cost to a documented entry point; the escape hatch is the published `ocx` bin, where genuine shell exports survive by proof. The gap that let the bad revision pass: the suite tested no-context credential handling and settings-hijack separately, never combined. It does now — a no-context ambient key must be absent after the merge that hijacks the destination. Reintroducing the narrowing fails 6 tests.
|
@Ingwannu @Wibias — requesting the security review this has been waiting on since 07-27. Surfaced by a triage sweep over every open issue and PR ( State against
Two things worth separating. The rebase is mechanical and mine to do — please don't spend review time on the conflicts. What is not unblocked by a rebase is the boundary decision itself, and that is the part I'm asking for: even a perfectly rebased branch cannot merge without this review. For context the PR body already makes explicit: this is the outbound listener question — whether the process answering on the port deserves to — and it was not superseded by #917, which resolved the inbound management principal. Reviewing it as a duplicate of #917 would miss the change. |
Closed #1045 on ancestry plus a green suite. Requested review on #936, #557, and #1018 - and found that #936, recorded as needing security review in four separate devlog rounds, had no reviewer assigned at all. Every round was right that it was blocked; none of them assigned anyone, so an administrative gap read as a technical one for nine days. The record also states why sixteen issues stayed open, including the four whose fix is provably on dev but whose reported symptom was never reproduced here. Closing on a shipped fix that was never shown to address the report is how an issue gets closed twice.
Rebase of #916 (@Ingwannu) onto current
dev, with the three defects a branch review found.Draft on purpose. This touches authentication, credential handling, GitHub workflows, and the durable launcher —
MAINTAINERS.mdrequires explicit security review for exactly that surface. The review below is an audit, not that review, and an agent does not substitute for it. Do not merge on my say-so.The defects are real on current
devConfirmed by direct probe rather than by reading the PR description:
src/adapters/google.tsbuilds the request host fromprovider.locationwith no validation, then attaches an ADC bearer. A location ofattacker.example:443/capture#yields authorityattacker.example— I reproduced this againstdev.overrideBunPath()rereadsOPENCODEX_BUN_PATHafter Bun has loaded project dotenv, so everydurableBunRuntime()caller — service, shim, WinSW, tray — can persist a repository-chosen executable.ANTHROPIC_BASE_URLsurvived credential stripping and reached Claude, which can redirect an OAuth-bearing request.src/oauth/health.tsattached the admin token after only a forgeable/healthzidentity check.None of these were superseded by #917. That PR resolved the inbound management principal — who may call the star route. This is the outbound listener: whether the thing answering on the port deserves the token. Different boundary.
What the review changed
src/cli/claude.tsno-context fallback — narrowed. As submitted, a launch with no trusted launcher context deleted all three ambient Anthropic slots.bun src/cli/index.tsis a documented entry point (structure/01_runtime.md:9) and has no launcher context, so a user with a shell-exportedANTHROPIC_API_KEYsimply lost it.The two slot classes are not symmetric.
ANTHROPIC_BASE_URLstays fail-closed — a dotenv-only destination plus subscription auth is precisely how the OAuth bearer leaves for a repository-chosen host, and losing a legitimate custom destination costs a flag rather than an account. Credentials are preserved: the destination is already pinned by the time they are read, so stripping them defends against a project file that could equally well have supplied the key being blamed. The test that mandated the old behavior is replaced by two that pin the asymmetry.structure/06_docs-and-release.md— theci.ymlrow still described the pre-#899 hosted-Windows selector. Rewritten against the real job graph: four Linux shards plusgates, full macOS, Windows only at the shipping boundary, and the aggregatecijob asserting Windows actually succeeded there.structure/01_runtime.mddecision log — recorded the rejected fail-closed-for-everything behavior; now describes the split.Rebase
One conflict,
src/server/index.ts, resolved keeping both #917'smanagementPrincipal()dispatch and this branch's attestation imports.Evidence
bun run test: 7602 pass / 0 fail across 505 filesbun x tsc --noEmitexit 0,bun run privacy:scanpassedAuthorship on the original commits is preserved. @Ingwannu — the substance is yours; the three changes above are the review response, and I would rather you disagree with them here than have them land silently.
Security review round — my narrowing was wrong
The adversarial review returned FAIL on the one change I made to @Ingwannu's behavior, and it was right.
I had preserved ambient credentials when no launcher context exists, reasoning that the destination is pinned before they are read, so a dotenv-supplied key could only ever reach the local proxy. That reasoning does not survive the subscription path.
CLAUDE_CODE_PROVIDER_MANAGED_BY_HOSTis only set when opencodex owns an auth token — asserting it otherwise logs a subscriber out (#253) — so Claude Code'ssettings.envmerge can still replaceANTHROPIC_BASE_URLafterbuildClaudeEnvreturns. A preserved key travels to that host.This repository already documents the destination residual for subscription mode, with a test asserting it so it cannot drift into an assumed guarantee. Preserving credentials would have quietly widened that documented residual into a credential leak.
So all three slots fail closed again without provenance, exactly as @Ingwannu submitted it. Direct
bun src/cli/index.tsloses ambient Anthropic values — a real cost to a documented entry point, and the honest trade. The escape hatch is running through the publishedocxbin, where genuine shell exports survive by proof.The gap that let my bad revision pass: the suite tested no-context credential handling and settings-hijack separately, never combined. It does now — a no-context ambient key must be absent after the merge that hijacks the destination. Reintroducing the narrowing fails 6 tests.
The review also cleared the rest: the Vertex validator is a strict lowercase 1–63 byte DNS label rejecting dots, Unicode, percent-encoding, fragments, paths, uppercase and overlong values while accepting every legitimate location, enforced on both the adapter and the management write path; local attestation binds a fresh 256-bit challenge plus PID and port, fails closed on malformed proofs, and compares timing-safely; the Bun launcher stamps before Bun starts on every documented invocation including the Windows
.cmdshim; and the rebase keptmanagementPrincipal()with the correcthandleManagementAPIarity.Updated:
bun run test7602 pass / 0 fail across 505 files, typecheck exit 0, privacy scan passed.Still draft, still needs maintainer security review. Two rounds of adversarial audit are not that review.